Limit video attachment size #383
Conversation
There was a problem hiding this comment.
Thanks for tackling this!
One suggestion about readability. When I added the check to make sure that the metadata.json wasn't empty, I originally wrote code that looked a lot like this, and Matteo suggested exiting early instead, in order to avoid nesting. #377. I think possibly the same kind of tweak could work here?
If you don't think so, feel free to ignore :-)
Scoop.js
Outdated
| @@ -948,14 +949,19 @@ export class Scoop { | |||
| const body = await readFile(`${this.captureTmpFolderPath}${file}`) | |||
| const isEntryPoint = false // TODO: Reconsider whether this should be an entry point. | |||
|
|
|||
| this.addGeneratedExchange(url, httpHeaders, body, isEntryPoint) | |||
| videoSaved = true | |||
| if (body.length) { | |||
There was a problem hiding this comment.
How would you feel about checking for if !body.length and then continueing, instead of setting videoSaved and then checking it?
There was a problem hiding this comment.
Thanks! Yes, let me overhaul this -- I was thinking the way I did it might not be idiomatic or concise.
There was a problem hiding this comment.
I think I can't take out your block, as that return is what prevents the summary from being created.
There was a problem hiding this comment.
Oh, or maybe I can move it up higher.
|
I'm not sure the changes I just made are exactly what you had in mind. I now |
Yeah! That's exactly what I had in mind! |
This PR limits video attachment size to 200MB by default, and makes it configurable. See Linear issue LIL-2879 for some discussion. Initially, I thought we would truncate the video attachment at the specified size, but this is likely to be fussy, and I think would introduce a dependency on ffmpeg. As it stands, a primary video on a page that is larger than the setting will not be saved as an attachment.
Additionally, this PR corrects one default, and updates the package macos-release, so that developers using Sequoia can continue their work :) Thanks to @matteocargnelutti for the tip on the package update, and to @rebeccacremona for consultation.